Register command routes independently of the application's routing style - #34
Conversation
Command route discovery hung off a decorator on `routing.loader.attribute.directory`, so it only ran when the application happened to load routes through that loader. The current Symfony skeleton's config/routes.yaml sets a `namespace`, which routes through Psr4DirectoryLoader instead, so the decorator was never invoked and command routes silently did not exist — no error, just 404s. Decorate `routing.loader` instead. That is the loader the router resolves to build its collection, and it is called exactly once for the root routing resource, so discovery runs regardless of how (or whether) the application declares its own routes. DelegatingLoader is not itself tagged `routing.loader`, so it is absent from the resolver nested imports go through and the decoration cannot recurse. Discovery moves out of the decorator into CommandRouteDiscovery, which scans the new `command_paths` setting (default `%kernel.project_dir%/src`, matching the previous hardcoded path) and skips paths that do not exist. Setting it to [] disables discovery for applications that prefer to import commands explicitly. CommandRouteDirectoryLoader existed and was unit-tested but was never registered as a service; it is now the directory-scanning loader behind discovery and is registered so `type: stixx_openapi_command.command_attributes` works from an application's routing config. The functional kernel imports every command explicitly, so it exercised the explicit-import path and never covered discovery. RouteDiscoveryTest adds a kernel that declares no routes at all; both of its tests fail against the old decoration target.
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Keep the reasoning that is not recoverable from the code — why routing.loader is the decoration point, why $inner is typed as Loader — and drop the rest.
RouteSpecificitySorter rebuilt the collection without copying resources, and the decorator added routes without them, so the router cache had nothing covering the scanned command directories. In debug mode a new or edited command DTO did not invalidate the route cache and stayed invisible until a manual cache:clear. The previous decorator lost resources the same way, but it only ran while the application's own AttributeDirectoryLoader was scanning src/, whose collection carried a resource for those files. Nothing scans the command paths now, so the resource has to come from discovery itself. Also carry route priorities through the decorator's copy, and correct the note on the $inner type: MicroKernelTrait calls getResolver() on this service, not import() — RoutingConfigurator gets a RoutingPhpFileLoader, not the decorator.
|
Self-review found a blocking issue; fixed in 9207b65. Discovered routes carried no cache resources. Since Worth noting this was made worse by the PR rather than inherited: the old decorator dropped resources through the same sorter, but it only ran while the application's own Also in this commit:
Tests: resource preservation is now pinned at all three levels (sorter, discovery, decorator) plus the functional kernel. I verified all four fail with the fix reverted. 219 tests pass, PHPStan max and PHP-CS-Fixer clean. Two minor points I did not change, happy to follow up if you want them:
|
Each command class is loaded into its own collection and those are merged with addCollection(), which overwrites by name. Two commands resolving to the same route name therefore cost one of them its endpoint, silently — the same class of failure this branch set out to remove. Without an operationId the name derives from the class short name, so Billing\CreateInvoiceCommand and Sales\CreateInvoiceCommand both resolve to command_createinvoicecommand. Track the owning class per route name in the class loader and throw, naming both classes. Loading the same class twice, via discovery and an explicit import, stays valid. Detection lives in the class loader rather than in discovery because AttributeDirectoryLoader already merges per-class collections, so a collision within one directory is gone before discovery sees it. Also record why import() is delegated rather than dropped.
|
Handled both remaining findings in 97e34c1. Route-name collisions now fail loudly. This turned out broader than the multi-path case I originally flagged. Detection lives in Worth calling out as a behaviour change: an application that today has a silent collision will now fail to compile instead of quietly losing an endpoint. That is the intent — it surfaces an existing latent bug rather than introducing one — but it can turn a working build red on upgrade. Documented in
221 tests, PHPStan max and PHP-CS-Fixer clean. |
1 participant
Follow-up to #33, and independent of it — both branch from
main.Problem
Command route discovery hung off
AttributeDirectoryLoaderDecorator, which decoratedrouting.loader.attribute.directory. Discovery therefore only ran as a side effect of the application happening to load routes through that specific loader.The current Symfony skeleton's
config/routes.yamldoes not:The
namespacekey routes this throughPsr4DirectoryLoader, notAttributeDirectoryLoader. The decorator is never invoked, no command routes are registered, and there is no error — endpoints simply 404. Working around it means adding a routing import purely to coax the right loader into running.Change
Decorate
routing.loaderinstead — FrameworkBundle'sDelegatingLoader, which is whatRouter::getRouteCollection()resolves and calls exactly once for the root routing resource. Discovery now runs regardless of how the application declares its routes, or whether it declares any.Two properties make this safe:
DelegatingLoaderis not itself taggedrouting.loader, so it is absent from the resolver that nested imports go through — the decoration cannot recurse.$augmentedlatch the old decorator used to avoid re-scanning.Supporting changes:
CommandRouteDiscovery.command_pathssetting, defaulting to['%kernel.project_dir%/src']— the path that was previously hardcoded. Non-existent paths are skipped;[]disables discovery entirely.CommandRouteDirectoryLoaderalready existed and was unit-tested, but was never registered as a service. It is now the directory loader behind discovery, and registering it makestype: stixx_openapi_command.command_attributesusable from an application's routing config.CommandRouteClassLoaderkeeps itsrouting.loadertag, so$routes->import(SomeCommand::class, 'attribute')still works.Tests
The functional kernel imports every command explicitly (
$routes->import(CreateBookCommand::class, 'attribute')), so it exercised the explicit-import path and never covered discovery — which is how this shipped broken.RouteDiscoveryTestadds a kernel that declares no routes, and asserts both that the routes are registered and that they serve requests. Verified against the old decoration target: both tests fail, the request test with a 404.216 tests pass; PHPStan and PHP-CS-Fixer are clean.
Compatibility
AttributeDirectoryLoaderDecoratoris removed. It is@internal, as are all route loaders per the README's stability policy. Applications that added a routing import to work around the old behaviour can keep it — those routes win over discovered ones.